Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RJS-2187: Add progress information to RealmProvider fallback. #6801

Merged
merged 9 commits into from
Aug 8, 2024

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Jul 19, 2024

@realm/devdocs

What, How & Why?

Solves #5459

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary
  • 🔔 Mention @realm/devdocs if documentation changes are needed

@gagik gagik linked an issue Jul 19, 2024 that may be closed by this pull request
@gagik gagik requested a review from kraenhansen July 22, 2024 07:04
COMPATIBILITY.md Outdated Show resolved Hide resolved
if (progressValues instanceof Array) {
const sendProgress = () => {
// Uses act as this causes a component state update.
act(() => callback(progressValues[progressIndex]));
Copy link
Member

@kraenhansen kraenhansen Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would be better to be moved into the calling callback instead 🤔 To keep this mock a bit cleaner (and independent of @testing-library/react-native).

Copy link
Contributor Author

@gagik gagik Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the calling callback in this case is RealmProvider so I do not have much control over that callback sadly; I could create the option to let the user define a function wrapper but seems too much for a convenience method

},
);

const delayedRealmOpen = jest.spyOn(Realm, "open");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(How) is this mock restored? Perhaps this side-effect could be moved into the test where you could also call jest.restoreAllMocks() to avoid this state bleeding into other tests.

Copy link
Contributor Author

@gagik gagik Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been running the cleaning but I agree it's not intuitive that this side effect exists. Maybe even extending this jest with like jest.mockRealmOpen() could be the most consistent with rest of jest mock API to have these mock helpers (and then intuitively jest.restoreAllMocks() cleans them).

Alternatively we wrap them in a hook which includes a clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like extending jest isn't the norm as it is with extending expect so I guess we either can define a hook with cleanup built in (although then if we have multiple mocks we'd be running the cleanup too much) or use the function as is and just make sure to run the restoreAllMocks

packages/realm-react/CHANGELOG.md Show resolved Hide resolved
@gagik gagik merged commit 9e2b720 into main Aug 8, 2024
8 checks passed
@gagik gagik deleted the gagik/realm-fallback-process branch August 8, 2024 09:21
kraenhansen added a commit that referenced this pull request Aug 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RealmProvider fallback property should include progress notifications
3 participants